Use bytes::Bytes as the HTTP request body in HttpClient. #2485
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2485 +/- ##
=======================================
- Coverage 77.9% 77.9% -0.1%
=======================================
Files 123 123
Lines 22888 22891 +3
=======================================
Hits 17839 17839
- Misses 5049 5052 +3 ☔ View full report in Codecov by Sentry. |
|
Hello @cijothomas |
This will allow pooling of buffers on caller side compared to usage of Vec<u8>.
mstyura
force-pushed
the
http-client-request-body-bytes
branch
from
2e19563 to
e0156e6
Compare
| /// | ||
| /// Returns an error if it can't connect to the server or the request could not be completed, | ||
| /// e.g. because of a timeout, infinite redirects, or a loss of connection. | ||
| async fn send_bytes(&self, request: Request<Bytes>) -> Result<Response<Bytes>, HttpError>; |
There was a problem hiding this comment.
Just so I understand you want to create a pool of Bytes and reuse them across different send/send_bytes call?
There was a problem hiding this comment.
Yes, exactly, serialization of spans to export and send to Datadog is one of the largest memory allocation source of backend service I'm working on. I want to reuse buffers to serialize and sending over http between calls to exports.
There was a problem hiding this comment.
@mstyura - will this enable buffer reuse in opentelemetry-otlp as well? It doesn't seem to be part of this PR, so ensuring.
There was a problem hiding this comment.
@lalitb I didn't have such plans initially, as I see proposed API is used by oltp and zipkin inside this repo, so they could leverage as well from buffer pooling. But as I don't use them in production I don't understand how heavily they actually allocate memory compared to DataDog exported I initially wanted to fix.
Do you want me to implement pooling for oltp and zipkin? If so I think it's preferable to do it as separate MR.
| @@ -28,7 +28,7 @@ impl LogExporter for OtlpHttpClient { | |||
| .method(Method::POST) | |||
| .uri(&self.collector_endpoint) | |||
| .header(CONTENT_TYPE, content_type) | |||
| .body(body) | |||
| .body(body.into()) | |||
There was a problem hiding this comment.
I believe this simply takes ownership of the Vec's buffer and wraps it in a Bytes object? Just want to ensure there is no copy operation here.
This will allow pooling of buffers on caller side compared to usage of Vec.
Fixes
Lack of possibility to reuse buffers which are consumed by
HttpClientafter request has been sent.Design discussion issue
Should old function be kept with deprecated annotation and hence new function had to have a different name or should I just introduce breaking change by changing signature of a function?
Changes
I propose to change type of body of http request accepted by
HttpClienttrait fromVec<u8>tobytes::Bytes.This should unblock pooling of buffers which can be used both for serialization & being http request bodies.
Something like this (pseudo-code to get the idea):
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes